-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf(python): Add __slots__
to most Polars classes
#13236
Conversation
@@ -43,7 +43,6 @@ def __get__(self, instance: NS | None, cls: type[NS]) -> NS | type[NS]: | |||
return self._ns | |||
|
|||
ns_instance = self._ns(instance) # type: ignore[call-arg] | |||
setattr(instance, self._accessor, ns_instance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this were not required even before, but it served as "cache" on instance of expression.
Now it gives error because instance does not have __dict__
and _accessor
goes straight to descriptor - and it's read only.
But since expressions are frequently recreated, then this "cache" were only working on chained namespace access like
pl.col("foo").namespace_foo.func().namespace_foo.other_func()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd appreciate if @alexander-beedie could sign off on this change before this gets merged. I don't really see exactly what is going on here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out this did have user impact:
#14851
For this reason, and some related issues popping up, I will be reverting this PR.
Thanks for the PR. Could you provide some data on the performance impact of this change? I think I'm in favor of this change, but it would be nice to have some numbers to support it. About subclassing: we don't support this. That test is from a while ago and we no longer guarantee preserving the class on calling a method. |
It's hard to measure exactly, because of variety of possible use cases. But speedup is small, which is anticipated. In my production use case constructing pl.Expr and pl.LazyFrame with all the glue takes ~20% of request runtime, while collecting them only about 10% because dataframes are pretty small (100-2000 rows). Anyway here is some synthetic bench. upstream
this PR
Also docs are failing to build because of this, because sphinx tries to get |
Ready to be merged or closed 😐 |
If you can rebase against the latest code I'll see about reviewing this 👌 |
8851dff
to
2f3219b
Compare
@alexander-beedie |
Does this prevent monkey patching? I use Also, what about this syntax |
@deanm0000
Also suggesting to take a look at pipe method on expression for this, it also plays well with typing.
|
2f3219b
to
aa8e5c5
Compare
__slots__
to Expr
, LazyFrame
, and various other classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I left some comments.
I think Series
and DataFrame
can also get a __slots__
attribute. We do not guarantee subclass preservation, so I removed the related tests in another PR.
The way I see it, the biggest blocker for this PR for now is the Sphinx issue, which we should be able to address. Or we could add slots for these namespace classes later.
@@ -43,7 +43,6 @@ def __get__(self, instance: NS | None, cls: type[NS]) -> NS | type[NS]: | |||
return self._ns | |||
|
|||
ns_instance = self._ns(instance) # type: ignore[call-arg] | |||
setattr(instance, self._accessor, ns_instance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd appreciate if @alexander-beedie could sign off on this change before this gets merged. I don't really see exactly what is going on here.
py-polars/polars/expr/array.py
Outdated
if not BUILDING_SPHINX_DOCS: | ||
__slots__ = ("_pyexpr",) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of this construction. I would expect the sphinx_accessor
to take care of this, but it doesn't. We should fix that directly rather than introducing this check everywhere.
It seems to be a problem with sphinx-autosummary-accessors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on this. Will try to look into this.
I've tried monkey-patching, but dynamically adding __dict__
is way harder when __slots__
are in place, because it modifies object layout.
Problem seems to be that autosummary makes objects into "modules" and sphinx expects module objects to have dict, so exception thrown from sphinx itself.
Maybe will try to make subclass with __dict__
dynamically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created pull request to handle this xarray-contrib/sphinx-autosummary-accessors#124
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great! They have not released in a year though, so I don't expect this to be picked up quickly.
I'd suggest removing the namespace slots for now - we can add them again when the issue is resolved.
Thx; will look at the weekend |
aa8e5c5
to
d04a35f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13236 +/- ##
==========================================
- Coverage 80.92% 80.78% -0.15%
==========================================
Files 1328 1326 -2
Lines 173446 173123 -323
Branches 2455 2455
==========================================
- Hits 140369 139865 -504
- Misses 32605 32771 +166
- Partials 472 487 +15 ☔ View full report in Codecov by Sentry. |
4ed1b8b
to
7da83bd
Compare
__slots__
to Expr
, LazyFrame
, and various other classes__slots__
to most Polars classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the namespace slots for now. Let's merge this and see if we get any user feedback on this. If not, that would be a good thing :)
@stinodego May I ask what the decision criteria for inclusion finally was? I'm really just curious because I considered something similar in scikit-learn and measurement showed no performance improvement at all, quite similar to #13236 (comment) did neither. |
I like that this clearly documents/enforces what the instance variables are. That is the most important thing for me. I found out later that this changes the behavior of class A:
__slots__ = ("x",)
print(getattr(A, "x", None))
# <member 'x' of 'A' objects> <-- without slots this would print None But besides that, I don't see any big downsides so might as well give this a try. There may be some users out there that hit a specific case where the potential performance benefits actually help them. |
@stinodego Thanks for the explanation. |
Closes #13198
In this PR are "safe" additions of
__slots__
. Mostly for classes that are not usually subclassed by users -pl.Expr
, its subclasses and build in namespaces.polars.datatypes.classes
also really good candidates, since they are instantiated now, but not included in this PR (yet?).Adding
__slots__
for LazyFrame (andDataFrame
/Series
) turned out to be a breaking change because of subclassing.Using
ldf.__class__ = SubClassedLazyFrame
, like in test_preservation_of_subclasses is an error because of different object layouts. And even if subclass were to define__slots__
- they can only be empty, without additional attributes on instance to keep this assigment valid.That can't be solved without specific conversion mechanism to subclasses to properly create a new instance (like
pl.LazyFrame.as_subclass(SubClassedLazyFrame, *args, **kwargs)
) where_ldf
attribute itself would be passed/assigned to new instance of subclass.Also using
ldf.__class__ = SubClassedLazyFrame
a dirty hack anyway and if polars were to properly support subclassing of frames, I think something similar should be in place - then__slots__
on them can be added safely.Deeper description about
__slots__
and benefits\downsides can be found here.Also in issue were not mentioned memory usage benefits aside from slightly faster attribute access.
(sqlalchemy claimed 46% less memory usage for it's internal structures). Even though in latest python versions this gap should be smaller, due to more compact dict.